Conversation
|
""" Walkthrough종소리(벨) 커스텀 기능을 위한 데이터베이스 테이블, 엔티티, DTO, 레포지토리, 서비스, 예외 코드, 테스트 코드 및 API 문서가 추가·확장되었습니다. 벨 정보는 커스텀 타임박스와 연동되며, 벨의 시간과 횟수를 커스텀할 수 있도록 설계되었습니다. Changes
Sequence Diagram(s)sequenceDiagram
participant 사용자
participant Controller
participant CustomizeServiceV2
participant BellRepository
participant TimeBoxRepository
participant TableRepository
사용자->>Controller: 커스텀 테이블 저장/수정 요청 (벨 정보 포함)
Controller->>CustomizeServiceV2: save/updateTable 호출
CustomizeServiceV2->>TableRepository: 테이블 저장/조회/수정
CustomizeServiceV2->>TimeBoxRepository: 타임박스 저장/조회/삭제
CustomizeServiceV2->>BellRepository: 벨 저장/조회/삭제
CustomizeServiceV2-->>Controller: 응답 DTO 반환 (벨 정보 포함)
Controller-->>사용자: 결과 반환
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
📝 Test Coverage Report
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/main/java/com/debatetimer/repository/customize/BellRepository.java (1)
6-9: 최소한의 Repository 인터페이스입니다.현재 save 메소드만 정의되어 있어 요구사항에 맞습니다. 향후 벨 조회 기능이 필요할 경우 추가 메소드를 고려해보세요.
src/main/resources/db/migration/V9__add_bell_table.sql (1)
1-13: 데이터베이스 스키마가 올바르게 정의되었습니다.테이블 구조와 외래키 제약조건이 적절하게 설정되었습니다. 다만
time과count컬럼에 bigint를 사용했는데, 일반적인 사용 범위를 고려하면 int 타입으로도 충분할 것 같습니다.- time bigint not null, - count bigint not null, + time int not null, + count int not null,src/main/java/com/debatetimer/entity/customize/Bell.java (2)
43-47: 벨 시간 검증 로직을 개선하세요.현재
time >= 0을 허용하고 있지만, 0초에 벨이 울리는 것이 의미가 있는지 검토가 필요합니다. 일반적으로 벨은 양수 시간에 울리는 것이 자연스럽습니다.다음과 같이 수정을 고려해보세요:
private void validateTime(int time) { - if (time < 0) { + if (time <= 0) { throw new DTClientErrorException(ClientErrorCode.INVALID_BELL_TIME); } }
31-32: 필드 접근 제한자를 고려하세요.JPA 엔티티에서 필드를
private으로 선언하는 것이 캡슐화 관점에서 더 좋습니다.- private int time; - private int count; + private final int time; + private final int count;또는 최소한:
- private int time; - private int count; + private final int time; + private final int count;src/main/java/com/debatetimer/service/customize/CustomizeService.java (1)
104-104: 하드코딩된 값들을 상수로 정의하세요.벨 횟수(1, 2)와 경고 시간(30)이 하드코딩되어 있습니다. 이를 상수로 정의하여 유지보수성을 높이세요.
+private static final int WARNING_BELL_COUNT = 1; +private static final int FINISH_BELL_COUNT = 2; +private static final int WARNING_BELL_OFFSET_SECONDS = 30; // 사용 시: -new Bell(timeBox, timeBox.getTime(), 2) +new Bell(timeBox, timeBox.getTime(), FINISH_BELL_COUNT) -new Bell(timeBox, timeBox.getTime() - 30, 1) +new Bell(timeBox, timeBox.getTime() - WARNING_BELL_OFFSET_SECONDS, WARNING_BELL_COUNT)Also applies to: 114-114
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/main/java/com/debatetimer/dto/customize/response/BellResponse.java(1 hunks)src/main/java/com/debatetimer/dto/customize/response/CustomizeTableResponse.java(1 hunks)src/main/java/com/debatetimer/dto/customize/response/CustomizeTimeBoxResponse.java(1 hunks)src/main/java/com/debatetimer/entity/customize/Bell.java(1 hunks)src/main/java/com/debatetimer/exception/errorcode/ClientErrorCode.java(1 hunks)src/main/java/com/debatetimer/repository/customize/BellRepository.java(1 hunks)src/main/java/com/debatetimer/service/customize/CustomizeService.java(5 hunks)src/main/resources/db/migration/V9__add_bell_table.sql(1 hunks)src/test/java/com/debatetimer/controller/customize/CustomizeDocumentTest.java(9 hunks)src/test/java/com/debatetimer/entity/customize/BellTest.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/com/debatetimer/service/customize/CustomizeService.java (1)
src/main/java/com/debatetimer/exception/custom/DTClientErrorException.java (1)
DTClientErrorException(5-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-push
🔇 Additional comments (6)
src/main/java/com/debatetimer/dto/customize/response/BellResponse.java (1)
3-7: 간단하고 명확한 응답 DTO 구조입니다.Record 타입을 사용하여 불변성을 보장하고 있으며, 필드명이 명확합니다. 응답 DTO이므로 별도의 유효성 검사 어노테이션이 필요하지 않습니다.
src/main/java/com/debatetimer/exception/errorcode/ClientErrorCode.java (1)
69-70: 적절한 벨 유효성 검사 에러 코드가 추가되었습니다.에러 메시지가 명확하고 기존 패턴을 잘 따르고 있습니다. 벨 시간은 0 이상, 벨 카운트는 1 이상이라는 규칙이 합리적입니다.
src/test/java/com/debatetimer/entity/customize/BellTest.java (1)
10-29: 벨 엔티티 유효성 검사 테스트가 잘 구성되어 있습니다.네스티드 테스트 클래스 구조를 사용하여 테스트를 체계적으로 구성했고, 예외 타입과 메시지를 모두 검증하는 것이 좋습니다. 음수 시간과 0 카운트에 대한 검증 테스트가 적절합니다.
src/main/java/com/debatetimer/dto/customize/response/CustomizeTimeBoxResponse.java (1)
6-6: 벨 필드 추가가 적절히 구현되었습니다.새로운 벨 기능을 지원하기 위한 필드 추가와 생성자 수정이 올바르게 구현되었습니다.
Also applies to: 13-13, 19-19, 25-25
src/main/java/com/debatetimer/dto/customize/response/CustomizeTableResponse.java (1)
10-12: 생성자 리팩토링이 적절합니다.벨 기능을 지원하기 위해 생성자를 단순화한 것이 좋은 접근입니다. 변환 로직을 서비스 레이어로 이동시켜 책임을 명확히 분리했습니다.
src/test/java/com/debatetimer/controller/customize/CustomizeDocumentTest.java (1)
26-26: API 문서화가 잘 구현되었습니다.벨 기능에 대한 API 문서화가 모든 관련 엔드포인트에서 일관성 있게 추가되었고, 선택적 필드로 적절히 표시되었습니다. 테스트 데이터도 실제 사용 시나리오를 잘 반영하고 있습니다.
Also applies to: 93-95, 123-130, 228-230, 245-252, 342-344, 373-380, 483-485, 501-508
src/main/java/com/debatetimer/service/customize/CustomizeService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/debatetimer/service/customize/CustomizeService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/debatetimer/service/customize/CustomizeService.java
Outdated
Show resolved
Hide resolved
Test Results 76 files 76 suites 13s ⏱️ Results for commit 507e39f. ♻️ This comment has been updated with latest results. |
leegwichan
left a comment
There was a problem hiding this comment.
/noti 님, 어짜피 수정할거니까 간략하게 짜신 건 이해하는데요.
조회만 해도 알람이 늘어나는 건 좀;; 이러시면 구구한테 연락해서 리뷰해달라고 할겁니다.
| private void validateCount(int count) { | ||
| if (count <= 0) { | ||
| throw new DTClientErrorException(ClientErrorCode.INVALID_BELL_COUNT); | ||
| } | ||
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/java/com/debatetimer/service/customize/CustomizeService.java (1)
109-111: 응답 구성에서 음수 시간 검증이 누락되었습니다.
getCustomizeTableResponse메서드에서 경고 벨의 시간을 계산할 때 음수 검증이 없습니다.saveBell메서드에서는 30초 이상인 경우만 처리하도록 필터링하고 있지만, 응답 구성에서는 동일한 검증이 없어 일관성 문제가 있습니다.다음과 같이 수정하세요:
if (savedTable.isWarningBell()) { - bell.add(new BellResponse(timeBox.getTime() - 30, 1)); + if (timeBox.getTime() >= 30) { + bell.add(new BellResponse(timeBox.getTime() - 30, 1)); + } }
🧹 Nitpick comments (1)
src/main/java/com/debatetimer/service/customize/CustomizeService.java (1)
99-123: 메서드 복잡성 개선이 필요합니다.
getCustomizeTableResponse메서드가 여러 책임을 가지고 있어 단일 책임 원칙을 위반하고 있습니다. 벨 정보 구성 로직을 별도 메서드로 분리하는 것을 권장합니다.다음과 같이 벨 정보 구성 로직을 분리할 수 있습니다:
private List<BellResponse> createBellResponses(CustomizeTimeBox timeBox, CustomizeTableEntity table) { if (!timeBox.getBoxType().isNotTimeBased() || (!table.isFinishBell() && !table.isWarningBell())) { return null; } List<BellResponse> bells = new ArrayList<>(); if (table.isWarningBell() && timeBox.getTime() >= 30) { bells.add(new BellResponse(timeBox.getTime() - 30, 1)); } if (table.isFinishBell()) { bells.add(new BellResponse(timeBox.getTime(), 2)); } return bells; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/com/debatetimer/entity/customize/Bell.java(1 hunks)src/main/java/com/debatetimer/exception/errorcode/ClientErrorCode.java(2 hunks)src/main/java/com/debatetimer/repository/customize/BellRepository.java(1 hunks)src/main/java/com/debatetimer/service/customize/CustomizeService.java(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/debatetimer/exception/errorcode/ClientErrorCode.java
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/com/debatetimer/service/customize/CustomizeService.java (1)
src/main/java/com/debatetimer/exception/custom/DTClientErrorException.java (1)
DTClientErrorException(5-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-push
🔇 Additional comments (4)
src/main/java/com/debatetimer/entity/customize/Bell.java (1)
17-56: 엔티티 설계와 검증 로직이 적절합니다.Bell 엔티티의 구현이 요구사항을 잘 반영하고 있습니다. 특히 생성자에서 time과 count에 대한 검증을 수행하여 데이터 무결성을 보장하고 있으며, MAX_BELL_COUNT 상수를 통해 벨 횟수 제한을 명확히 하고 있습니다.
src/main/java/com/debatetimer/repository/customize/BellRepository.java (1)
8-15: 리포지토리 인터페이스 설계가 적절합니다.필요한 CRUD 메서드들이 명확하게 정의되어 있으며, Spring Data의 규칙에 따라 올바르게 구현되어 있습니다.
src/main/java/com/debatetimer/service/customize/CustomizeService.java (2)
125-147: 벨 저장 로직이 올바르게 구현되었습니다.경고 벨의 경우 30초 이상인 시간박스만 처리하도록 필터링하여 음수 시간 문제를 방지하고 있습니다. 마감 벨과 경고 벨을 구분하여 적절한 타입으로 저장하고 있습니다.
149-157: 벨 삭제 로직이 적절합니다.기존 벨 엔티티들을 정리하는 로직이 올바르게 구현되어 있습니다. 시간 기반이 아닌 타임박스에 대해서만 처리하도록 필터링하고 있습니다.
|
/noti |
|
/noti |
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (2)
src/main/java/com/debatetimer/dto/customize/request/CustomizeTimeBoxCreateRequest.java (2)
39-46: 벨 데이터 매핑 로직 추가 필요
CustomizeTimeBoxCreateRequest.toTimeBox메서드에서 요청된bell필드를 무시하고 있으며, 서비스 레이어에서도BellRequest를 처리하는 로직이 전혀 없습니다.
아래 항목을 반영해 수정이 필요합니다:
- CustomizeTimeBoxCreateRequest.toTimeBox에서
List<BellRequest> bell을 도메인 객체(CustomizeTimeBox)에 매핑하도록 변경- CustomizeService.save/updateTable 등에서 DTO의 BellRequest를 Bell 엔티티로 변환 후 저장하거나,
CustomizeTimeBox생성 직후 Bell 저장 로직 추가예시(diff):
public CustomizeTimeBox toTimeBox(CustomizeTable customizeTable, int sequence) { - if (boxType.isTimeBased()) { - return new CustomizeTimeBox(new CustomizeTableEntity(customizeTable), sequence, stance, speechType, - boxType, timePerTeam, timePerSpeaking, speaker); - } - return new CustomizeTimeBox(new CustomizeTableEntity(customizeTable), sequence, stance, speechType, boxType, - time, speaker); + CustomizeTimeBox box; + if (boxType.isTimeBased()) { + box = new CustomizeTimeBox(new CustomizeTableEntity(customizeTable), sequence, + stance, speechType, boxType, timePerTeam, timePerSpeaking, speaker); + } else { + box = new CustomizeTimeBox(new CustomizeTableEntity(customizeTable), sequence, + stance, speechType, boxType, time, speaker); + } + // BellRequest → Bell 도메인 매핑 + if (bell != null) { + bell.forEach(b -> box.addBell(new BellEntity(box, b.time(), b.count()))); + } + return box; }
39-46: toTimeBox 메서드에서 벨 데이터 처리 누락새로 추가된
bell필드가toTimeBox메서드에서 처리되지 않고 있습니다. 이로 인해 벨 데이터가 엔티티 변환 과정에서 손실될 수 있습니다.public CustomizeTimeBox toTimeBox(CustomizeTable customizeTable, int sequence) { if (boxType.isTimeBased()) { return new CustomizeTimeBox(new CustomizeTableEntity(customizeTable), sequence, stance, speechType, - boxType, timePerTeam, timePerSpeaking, speaker); + boxType, timePerTeam, timePerSpeaking, speaker, bell); } return new CustomizeTimeBox(new CustomizeTableEntity(customizeTable), sequence, stance, speechType, boxType, - time, speaker); + time, speaker, bell); }
♻️ Duplicate comments (3)
src/test/java/com/debatetimer/service/customize/CustomizeServiceTest.java (3)
109-111: 다른 테스트 메서드에서도 동일한 테스트 데이터 불일치가 발생합니다.업데이트 테스트에서도 동일한 문제가 반복되고 있습니다.
125-129: 업데이트 테스트에서도 벨 검증 로직이 부정확합니다.저장 테스트와 동일한 문제가 발생하고 있습니다.
205-207: 마지막 테스트 케이스에서도 동일한 테스트 데이터 문제가 있습니다.모든 테스트에서 일관된 벨 설정을 사용하거나, 각 테스트의 목적에 맞는 검증 로직을 구현해야 합니다.
🧹 Nitpick comments (4)
src/test/java/com/debatetimer/service/customize/CustomizeServiceTest.java (4)
57-61: 벨 검증 로직 개선각 타임박스에 대해 정확히 2개의 벨이 있는지 검증하는 로직이 추가되었습니다. 하지만 하드코딩된 값 대신 실제 입력 데이터를 기반으로 검증하는 것이 더 나을 것 같습니다.
다음과 같이 개선할 수 있습니다:
() -> { for (CustomizeTimeBox timeBox : foundTimeBoxes) { - assertThat(bellRepository.findByCustomizeTimeBox(timeBox)).hasSize(2); + // 첫 번째 타임박스: 1개 벨, 두 번째 타임박스: 2개 벨 + int expectedBellCount = timeBox.getSequence() == 1 ? 1 : 2; + assertThat(bellRepository.findByCustomizeTimeBox(timeBox)).hasSize(expectedBellCount); } }
125-129: 일관된 벨 검증 로직업데이트 테스트에서도 동일한 벨 검증 로직이 사용되고 있습니다. 앞서 언급한 개선사항이 여기에도 적용될 수 있습니다.
144-146: 예외 테스트 케이스에서도 동일한 테스트 데이터를 사용하고 있습니다.예외 상황을 테스트하는 케이스에서도 일관성 있는 테스트 데이터를 사용하는 것이 좋습니다.
164-166: 동시성 테스트에서도 동일한 패턴이 반복됩니다.테스트 데이터의 일관성을 위해 모든 테스트에서 동일한 벨 설정 패턴을 사용하는 것을 고려해보세요.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/java/com/debatetimer/dto/customize/request/BellRequest.java(1 hunks)src/main/java/com/debatetimer/dto/customize/request/CustomizeTimeBoxCreateRequest.java(2 hunks)src/main/java/com/debatetimer/service/customize/CustomizeService.java(4 hunks)src/test/java/com/debatetimer/controller/customize/CustomizeDocumentTest.java(15 hunks)src/test/java/com/debatetimer/service/BaseServiceTest.java(2 hunks)src/test/java/com/debatetimer/service/customize/CustomizeServiceTest.java(8 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/main/java/com/debatetimer/dto/customize/request/BellRequest.java
- src/test/java/com/debatetimer/service/BaseServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/test/java/com/debatetimer/controller/customize/CustomizeDocumentTest.java
- src/main/java/com/debatetimer/service/customize/CustomizeService.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-push
🔇 Additional comments (8)
src/main/java/com/debatetimer/dto/customize/request/CustomizeTimeBoxCreateRequest.java (3)
26-27: 벨 필드 추가 확인됨새로운
bell필드가 올바르게 추가되었습니다.@Nullable어노테이션으로 선택적 구성이 가능하도록 설정되었습니다.
39-46: toTimeBox는 벨 처리를 책임지지 않습니다
벨 저장·삭제 로직은CustomizeService의 saveBell/deleteBell에서 이미 수행되고 있어, DTO → Entity 변환만 하는toTimeBox메서드에 벨 매핑을 추가할 필요가 없습니다.Likely an incorrect or invalid review comment.
26-27: 벨 필드 추가 승인벨 기능을 위한 필드 추가가 적절하게 구현되었습니다.
src/test/java/com/debatetimer/service/customize/CustomizeServiceTest.java (5)
10-10: BellRequest 임포트 추가 확인됨벨 기능을 위한
BellRequest임포트가 올바르게 추가되었습니다.
43-45: 벨 테스트 데이터 구조 양호현실적인 벨 구성을 사용하여 테스트 데이터를 설정했습니다. 90초에 1번, 120초에 2번의 벨 설정이 적절합니다.
10-10: BellRequest 임포트가 추가되어 벨 기능 테스트를 지원합니다.벨 기능 테스트를 위해 필요한 임포트가 적절히 추가되었습니다.
10-10: Import 추가 승인BellRequest import가 적절하게 추가되었습니다.
43-45: 테스트 데이터에 벨 정보 추가 승인테스트 케이스에 벨 정보가 적절하게 추가되었습니다.
| @Nullable | ||
| List<BellRequest> bell, |
There was a problem hiding this comment.
새로운 bell 필드가 추가되었지만 변환 로직에서 처리되지 않습니다.
bell 필드가 요청 DTO에 추가되었지만, toTimeBox 메서드에서 이 데이터를 처리하지 않고 있습니다. 이로 인해 클라이언트가 전송한 벨 설정 정보가 손실될 수 있습니다.
🤖 Prompt for AI Agents
In
src/main/java/com/debatetimer/dto/customize/request/CustomizeTimeBoxCreateRequest.java
around lines 26 to 27, the new 'bell' field is added but not handled in the
'toTimeBox' method. Update the 'toTimeBox' method to include logic that converts
and assigns the 'bell' list from the request DTO to the corresponding field in
the TimeBox entity, ensuring the bell settings from the client are preserved.
| () -> { | ||
| for (CustomizeTimeBox timeBox : foundTimeBoxes) { | ||
| assertThat(bellRepository.findByCustomizeTimeBox(timeBox)).hasSize(2); | ||
| } | ||
| } |
There was a problem hiding this comment.
벨 저장 검증 로직에 문제가 있습니다.
모든 timeBox에 대해 2개의 벨이 저장되었는지 검증하고 있지만, 테스트 데이터에서는 첫 번째 timeBox에 1개, 두 번째 timeBox에 2개의 벨이 설정되어 있습니다.
🤖 Prompt for AI Agents
In src/test/java/com/debatetimer/service/customize/CustomizeServiceTest.java
around lines 57 to 61, the test incorrectly asserts that each timeBox has 2
bells saved, but the test data has 1 bell for the first timeBox and 2 bells for
the second. Update the assertions to check the correct expected number of bells
for each timeBox individually, matching the test data setup.
벨 개수 검증 로직 오류
테스트 데이터와 assertion 간의 불일치가 있습니다. 첫 번째 타임박스는 1개의 벨만 가지고 있는데 모든 타임박스에 대해 2개의 벨이 있다고 검증하고 있습니다.
-() -> {
- for (CustomizeTimeBox timeBox : foundTimeBoxes) {
- assertThat(bellRepository.findByCustomizeTimeBox(timeBox)).hasSize(2);
- }
-}
+() -> {
+ assertThat(bellRepository.findByCustomizeTimeBox(foundTimeBoxes.get(0))).hasSize(1);
+ assertThat(bellRepository.findByCustomizeTimeBox(foundTimeBoxes.get(1))).hasSize(2);
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| () -> { | |
| for (CustomizeTimeBox timeBox : foundTimeBoxes) { | |
| assertThat(bellRepository.findByCustomizeTimeBox(timeBox)).hasSize(2); | |
| } | |
| } | |
| () -> { | |
| assertThat(bellRepository.findByCustomizeTimeBox(foundTimeBoxes.get(0))).hasSize(1); | |
| assertThat(bellRepository.findByCustomizeTimeBox(foundTimeBoxes.get(1))).hasSize(2); | |
| } |
🤖 Prompt for AI Agents
In src/test/java/com/debatetimer/service/customize/CustomizeServiceTest.java
around lines 57 to 61, the assertion incorrectly expects 2 bells for every
CustomizeTimeBox, but the first time box only has 1 bell. Adjust the assertion
logic to verify the bell count according to each time box's actual expected
number of bells, ensuring the test data and assertions match correctly.
| 120, List.of(new BellRequest(90, 1)), 60, null, "발언자1"), | ||
| new CustomizeTimeBoxCreateRequest(Stance.PROS, "입론2", CustomizeBoxType.NORMAL, | ||
| 120, 60, null, "발언자2") | ||
| 120, List.of(new BellRequest(90, 1), new BellRequest(120, 2)), 60, null, "발언자2") |
There was a problem hiding this comment.
테스트 데이터와 어설션 로직이 일치하지 않습니다.
첫 번째 timeBox는 1개의 벨만 설정되어 있고, 두 번째 timeBox는 2개의 벨이 설정되어 있습니다. 하지만 어설션에서는 모든 timeBox에 대해 2개의 벨을 기대하고 있어 테스트가 실패할 수 있습니다.
테스트 데이터를 일관되게 수정하거나 어설션 로직을 개별적으로 검증하도록 변경해야 합니다:
-() -> assertThat(bellRepository.findByCustomizeTimeBox(timeBox)).hasSize(2);
+() -> {
+ List<CustomizeTimeBox> timeBoxes = foundTimeBoxes.stream().sorted(Comparator.comparing(CustomizeTimeBox::getSequence)).toList();
+ assertThat(bellRepository.findByCustomizeTimeBox(timeBoxes.get(0))).hasSize(1);
+ assertThat(bellRepository.findByCustomizeTimeBox(timeBoxes.get(1))).hasSize(2);
+}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/test/java/com/debatetimer/service/customize/CustomizeServiceTest.java
around lines 43 to 45, the test data defines the first timeBox with one bell and
the second with two bells, but the assertions expect two bells for all timeBoxes
causing test failures. To fix this, either update the test data so all timeBoxes
have two bells or adjust the assertion logic to verify the number of bells
individually per timeBox according to the test data.
| () -> { | ||
| for (CustomizeTimeBox timeBox : updatedTimeBoxes) { | ||
| assertThat(bellRepository.findByCustomizeTimeBox(timeBox)).hasSize(2); | ||
| } | ||
| } |
There was a problem hiding this comment.
벨 개수 검증 로직 오류 (중복)
이 부분도 동일한 문제가 있습니다. 각 타임박스마다 다른 개수의 벨이 있는데 모두 2개라고 검증하고 있습니다.
-() -> {
- for (CustomizeTimeBox timeBox : updatedTimeBoxes) {
- assertThat(bellRepository.findByCustomizeTimeBox(timeBox)).hasSize(2);
- }
-}
+() -> {
+ assertThat(bellRepository.findByCustomizeTimeBox(updatedTimeBoxes.get(0))).hasSize(1);
+ assertThat(bellRepository.findByCustomizeTimeBox(updatedTimeBoxes.get(1))).hasSize(2);
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| () -> { | |
| for (CustomizeTimeBox timeBox : updatedTimeBoxes) { | |
| assertThat(bellRepository.findByCustomizeTimeBox(timeBox)).hasSize(2); | |
| } | |
| } | |
| () -> { | |
| - for (CustomizeTimeBox timeBox : updatedTimeBoxes) { | |
| - assertThat(bellRepository.findByCustomizeTimeBox(timeBox)).hasSize(2); | |
| - } | |
| + assertThat(bellRepository.findByCustomizeTimeBox(updatedTimeBoxes.get(0))).hasSize(1); | |
| + assertThat(bellRepository.findByCustomizeTimeBox(updatedTimeBoxes.get(1))).hasSize(2); | |
| } |
🤖 Prompt for AI Agents
In src/test/java/com/debatetimer/service/customize/CustomizeServiceTest.java
around lines 125 to 129, the test incorrectly asserts that each CustomizeTimeBox
has exactly 2 bells, but the number of bells varies per time box. Update the
assertions to check for the correct expected bell count for each specific
CustomizeTimeBox instead of assuming a fixed size of 2.
coli-geonwoo
left a comment
There was a problem hiding this comment.
/noti
비토, 고생많았습니다. 커찬이 리뷰 남김부분에 대해서는 prod 환경에만 영향을 주지 않는 마이그레이션 과정에서 치코와 충분히 협의한 내용이라면 나쁘지 않을 것 같아요.
몇가지 리뷰남겼으니 확인부탁합니다!
| @Entity | ||
| @Getter | ||
| @NoArgsConstructor(access = AccessLevel.PROTECTED) | ||
| public class Bell { |
There was a problem hiding this comment.
[사소한 제안]
아무래도 리팩터링 방향은 나중에 정한다 하더라도 네이밍은 Entity와 도메인이 분리되는 기조를 유지하면 좋을 것 같은데 BellEntity로 명명하는 것은 어떨까 제안합니다.
| return new CustomizeTimeBoxes(savedTimeBoxes); | ||
| } | ||
|
|
||
| @NotNull |
There was a problem hiding this comment.
[질문]
요거 메서드 위의 @NotNull은 어떤 의미인가요?
There was a problem hiding this comment.
저게 뭐지...? 수정했습니다.
| return new CustomizeTableResponse(savedTable.toDomain(), customizeTimeBoxResponses); | ||
| } | ||
|
|
||
| private void saveBell(CustomizeTableEntity savedTable, CustomizeTimeBoxes savedCustomizeTimeBoxes) { |
There was a problem hiding this comment.
[제안]
과도기적으로 saveBell과 deleteBell을 이렇게 처리하는 것에 대해 괜찮다고 생각해요.
2가지 제안을 드리고 싶어요.
-
아마, 여러까지 bell은 timeBox와 동일한 생명주기를 지니기 때문에 한번에 delete될 것 같은데 deleteAllInBatch로 처리하는 건 어떨까 싶어요.
-
추후에 반영할
반복문 쿼리를 유지한다면 원자적 동작을 위해@Transactional을 붙여주어야 한다고 생각합니다.
There was a problem hiding this comment.
1번은 반영했고 2번은 불필요해서 반영하지 않았습니다.
| create table bell | ||
| ( | ||
| id bigint auto_increment, | ||
| time bigint not null, |
There was a problem hiding this comment.
[제안]
- time은 아마 SQL에 예약어로 오인될 수 있을 것 같아서 DB 컬럼에 있어서는 다른 용어 사용하는 것은 어떤가요?
-> ring_time, ring_at, bell_time 등등도 괜찮을 것 같아요.
| } | ||
|
|
||
| @Test | ||
| void 벨_횟수는_1이상이어야_한다() { |
There was a problem hiding this comment.
최대 횟수 3회 이하여야 한다 테스트 하나 추가 부탁드립니다!
|
/noti |
leegwichan
left a comment
There was a problem hiding this comment.
/noti @unifolio0
getCustomizeTableResponse()로직 뭔지 잘 모르겠어요. 임시 로직이라 가독성 신경 안쓴 건 알겠는데, 어떤 목적으로 작성하셨는지는 알려주셔야... (주석이나 코멘트라도 남겨주셔야...)- 코드 레빗이 달아준 코멘트 중에 치명적인 부분이 꽤 많던데, 왜 그렇게 했는지 코멘트 달아주세요.
| () -> { | ||
| for (CustomizeTimeBox timeBox : updatedTimeBoxes) { | ||
| assertThat(bellRepository.findByCustomizeTimeBox(timeBox)).hasSize(2); | ||
| } | ||
| } |
There was a problem hiding this comment.
- Bell 검증 구문이 이게 최선입니까? CustomizeTimeBox id 가져와서 개수 검증이 좋지 않을까요?
- 토끼가 달아준 코멘트들이 굉장히 치명적으로 보이는데, 다 하나씩 답변 달아주실 수 있을까요? ex. 벨이 모두 2개인 이유, ...
| private CustomizeTimeBoxes saveTimeBoxesAndBells( | ||
| CustomizeTableCreateRequest tableCreateRequest, | ||
| CustomizeTable table | ||
| ) { | ||
| // TODO : 밑에 부분은 프론트 업데이트 후 주석 풀기 | ||
| /* | ||
| List<CustomizeTimeBoxCreateRequest> timeBoxCreateRequests = tableCreateRequest.table(); | ||
| List<CustomizeTimeBoxResponse> timeBoxResponses = IntStream.range(0, timeBoxCreateRequests.size()) | ||
| .mapToObj(i -> createTimeBoxResponse(timeBoxCreateRequests.get(i), table, i + 1)) | ||
| .toList(); | ||
| new CustomizeTableResponse(table, timeBoxResponses); | ||
| */ | ||
|
|
||
| // TODO : 밑에 부분은 프론트 업데이트 후 삭제 예정 | ||
| CustomizeTimeBoxes customizeTimeBoxes = tableCreateRequest.toTimeBoxes(table); | ||
| List<com.debatetimer.entity.customize.CustomizeTimeBox> savedTimeBoxes = timeBoxRepository.saveAll( | ||
| List<CustomizeTimeBox> savedTimeBoxes = timeBoxRepository.saveAll( | ||
| customizeTimeBoxes.getTimeBoxes()); | ||
| return new CustomizeTimeBoxes(savedTimeBoxes); | ||
| } |
There was a problem hiding this comment.
작업 목표가 아래와 같이 생각하면 되나요?
- 저장할 때 finishBell, warningBell 에 따라 알람 저장한다.
- 조회할 때 finishBell, warningBell 에 따라 넣어서 보여준다.
그러면, 이미 'finishBell, warningBell 에 따라 알람 저장' 한 테이블은 알람이 중복되서 두 번 보여지는 것 아닌가요? (아직 FE에서 사용하지 않을 것이라서 상관은 없겠지만...)
|
|
||
| void delete(BellEntity bell); | ||
|
|
||
| void deleteAllByCustomizeTimeBoxIn(List<CustomizeTimeBox> customizeTimeBoxes); |
There was a problem hiding this comment.
[질문]
오... 이거 in절로 해서 한번에 찾아와지는 건가요? 신기..🤔
There was a problem hiding this comment.
처음 보는 쿼리 메소드라 궁금해서 그런데 이거 @Modifying, clearAutomatically, flush 설정 없이 영속성 컨텍스트 1차 캐시를 활용해 동작하는 것인가요? 아니면 deleteAllInBatch 처럼 영속성 컨텍스트 반영 없이 바로 DB에 쏘아지게 되는건가요?
There was a problem hiding this comment.
전자입니다. 다만 그로인해 delete 쿼리가 customizeTimeBoxes의 수만큼 나가게 되긴하지만 악의적 유저의 요청을 제외하곤 잘 처리될 것 같습니다.
악의적 유저가 많은 것 같긴한데... 일단 좀 더 고민해보죠
There was a problem hiding this comment.
낙낙 패거리에 비토가 포함되어 있는 거잖아요?
그렇다면 비토 패거리 아닐까요?
| CustomizeTableCreateRequest tableCreateRequest, | ||
| CustomizeTable table | ||
| ) { | ||
| // TODO : 밑에 부분은 프론트 업데이트 후 주석 풀기 |
| if (timeBox.getBoxType().isNotTimeBased() && (savedTable.isFinishBell() || savedTable.isWarningBell())) { | ||
| bell = new ArrayList<>(); | ||
| if (savedTable.isWarningBell()) { | ||
| bell.add(new BellResponse(timeBox.getTime() - 30, 1)); |
There was a problem hiding this comment.
만약에 timeBox 시간이 30초 이하라면 - 값이 넣어지게 되는데 해당 예외상황에 대해 치코와 합의한 부분인지 궁금합니다!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/com/debatetimer/service/customize/CustomizeServiceV2.java (1)
112-124: 타임 기반 박스에 대한 벨 요청 처리 개선 제안타임 기반 박스의 경우
bellRequests파라미터가 무시됩니다. API 일관성을 위한 설계로 보이나, 명확성을 위해 다음을 고려해보세요:
- 문서화 추가로 타임 기반 박스에서는 벨이 무시됨을 명시
- 또는 검증 로직 추가로 타임 기반 박스에 벨 요청이 있을 경우 경고
private CustomizeTimeBoxResponse createTimeBoxResponse(List<BellRequest> bellRequests, CustomizeTimeBox timeBox) { if (timeBox.getBoxType().isTimeBased()) { + // 타임 기반 박스는 벨을 지원하지 않음 return new CustomizeTimeBoxResponse(timeBox, null); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/com/debatetimer/dto/customize/response/CustomizeTableResponse.java(1 hunks)src/main/java/com/debatetimer/dto/customize/response/CustomizeTimeBoxResponse.java(2 hunks)src/main/java/com/debatetimer/service/customize/CustomizeServiceV2.java(1 hunks)src/test/java/com/debatetimer/service/customize/CustomizeServiceTest.java(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/com/debatetimer/dto/customize/response/CustomizeTableResponse.java
- src/test/java/com/debatetimer/service/customize/CustomizeServiceTest.java
- src/main/java/com/debatetimer/dto/customize/response/CustomizeTimeBoxResponse.java
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/com/debatetimer/service/customize/CustomizeServiceV2.java (2)
src/main/java/com/debatetimer/exception/custom/DTClientErrorException.java (1)
DTClientErrorException(5-10)src/main/java/com/debatetimer/service/customize/CustomizeService.java (1)
Service(18-84)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-push
🔇 Additional comments (5)
src/main/java/com/debatetimer/service/customize/CustomizeServiceV2.java (5)
26-33: 적절한 의존성 주입 구조입니다.Spring의 표준 패턴을 잘 따르고 있으며, 새로운
BellRepository가 적절하게 추가되었습니다.
34-40: save 메서드가 깔끔하게 구현되었습니다.테이블 저장 후 타임박스와 벨을 저장하는 로직이 잘 분리되어 있습니다.
54-67: updateTable 메서드의 삭제 순서가 적절합니다.FK 제약을 고려하여 벨을 먼저 삭제하고 타임박스를 삭제하는 순서가 올바릅니다.
92-101: saveTimeBoxesAndBells 메서드가 잘 구조화되었습니다.IntStream을 활용한 시퀀스 처리와 함수형 스타일이 깔끔합니다. PR 설명에서 언급한 메서드 길이 우려와 달리, 적절히 분리되어 있습니다.
26-141: 전체적인 서비스 구조가 우수합니다.PR 설명에서 메서드 길이에 대한 우려를 표현하셨지만, 실제 구현은 다음과 같이 잘 설계되었습니다:
- 각 public 메서드가 명확한 책임을 가짐
- private 헬퍼 메서드들이 적절히 분리됨
- 트랜잭션 경계가 올바르게 설정됨
- 삭제 순서가 FK 제약을 고려함
향후 변경이 예상된다고 하셨지만, 현재 구조도 충분히 유지보수하기 좋습니다.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/com/debatetimer/repository/customize/BellRepository.java (1)
14-14: 성능 최적화를 위한 배치 삭제 고려해보세요.과거 코멘트에서 언급된 대로, 현재
deleteAllByCustomizeTimeBoxIn메서드는 영속성 컨텍스트를 통해 동작하여 CustomizeTimeBox 수만큼 개별 DELETE 쿼리가 실행됩니다. 대량 데이터 처리 시 성능 이슈가 발생할 수 있습니다.다음과 같은 대안을 고려해보세요:
+@Modifying +@Query("DELETE FROM BellEntity b WHERE b.customizeTimeBox IN :customizeTimeBoxes") +void deleteAllByCustomizeTimeBoxInBatch(List<CustomizeTimeBox> customizeTimeBoxes);또는 JPA의 배치 삭제 기능 활용:
+void deleteAllByCustomizeTimeBoxIn(List<CustomizeTimeBox> customizeTimeBoxes); + +@Modifying +@Query("DELETE FROM BellEntity b WHERE b.customizeTimeBox.id IN :timeBoxIds") +void deleteAllByCustomizeTimeBoxIds(List<Long> timeBoxIds);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/com/debatetimer/repository/customize/BellRepository.java(1 hunks)src/main/resources/db/migration/V10__add_bell_table.sql(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/resources/db/migration/V10__add_bell_table.sql
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-push
🔇 Additional comments (1)
src/main/java/com/debatetimer/repository/customize/BellRepository.java (1)
8-17: 레포지토리 인터페이스 설계가 잘 되어 있습니다.Spring Data의 Repository 인터페이스를 적절히 확장하고 있으며, 메서드 명명도 Spring Data 컨벤션을 잘 따르고 있습니다. 종소리 커스텀 기능에 필요한 CRUD 작업들이 적절히 정의되어 있습니다.
|
/noti |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/test/java/com/debatetimer/service/customize/CustomizeServiceTest.java(6 hunks)src/test/java/com/debatetimer/service/customize/CustomizeServiceV2Test.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/com/debatetimer/service/customize/CustomizeServiceTest.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-push
🔇 Additional comments (5)
src/test/java/com/debatetimer/service/customize/CustomizeServiceV2Test.java (5)
1-31: 테스트 클래스 구조가 잘 설계되었습니다.임포트 구문이 적절하고 BaseServiceTest를 상속받아 테스트 인프라를 효과적으로 활용하고 있습니다.
33-62: 포괄적인 생성 테스트가 잘 작성되었습니다.벨 설정을 포함한 복합적인 테스트 시나리오가 적절하게 구성되어 있으며, 생성된 엔티티들의 검증도 철저합니다.
64-98: 조회 기능과 권한 검증이 적절합니다.정상 케이스와 예외 케이스 모두를 다루고 있으며, 벨 데이터 조회도 함께 검증하고 있습니다.
100-174: 업데이트 기능 테스트가 포괄적입니다.정상 업데이트, 권한 검증, 동시성 처리까지 모두 테스트하고 있어 견고한 테스트 커버리지를 제공합니다.
217-252: 삭제 기능과 캐스케이드 삭제가 적절히 테스트되었습니다.테이블 삭제 시 관련된 타임박스와 벨까지 모두 삭제되는지 검증하여 데이터 일관성을 보장하고 있습니다.
src/test/java/com/debatetimer/service/customize/CustomizeServiceV2Test.java
Show resolved
Hide resolved
coli-geonwoo
left a comment
There was a problem hiding this comment.
/noti
2차 리뷰 남겼으니 한번 답변만 달아주세요!
approve 하면 답변 그냥 넘길 것 같아 답변만 확인하고 approve 하겠습니다!
| customizeTimeBox.getSpeechType(), | ||
| customizeTimeBox.getBoxType(), | ||
| convertTime(customizeTimeBox), | ||
| null, |
There was a problem hiding this comment.
[추후 논의 부탁]
- 개인적으로 논리의 비일관성이 생기고 분기문이 발생하는 이유가 이 nullable 함 때문이라고 생각해요
ex) 일반 타임박스에는 벨이 담길 수 있으나, 커스텀은 null이어야 함 등등
Request에서 Dto to Domain을 할때 빈 리스트를 반환하거나 초기화 방식으로 논리 일관성을 가져가면 어떨까요? 그럼 비토가 쓴 서비스 코드도 null을 의식하지 않고 분기문없이 초기화가능할 것 같아서요
(아... 코틀린 배우는 중이라 그런게 코틀린 마렵네 😄 )
|
|
||
| void delete(BellEntity bell); | ||
|
|
||
| void deleteAllByCustomizeTimeBoxIn(List<CustomizeTimeBox> customizeTimeBoxes); |
| CustomizeTimeBoxes timeBoxes = timeBoxRepository.findTableTimeBoxes(tableEntity); | ||
| List<CustomizeTimeBoxResponse> timeBoxResponses = timeBoxes.getTimeBoxes() | ||
| .stream() | ||
| .map(this::getTimeBoxResponse) |
There was a problem hiding this comment.
[추후 반영 제안]
getTimeBoxResponse가 무언가 했는데
- DTO를 조회한다? -> 이거 아니었네? -> DTO를 만드는 거엿네
- 조회용 메서드인가 보다 -> Bell도 저장하고 있잖아?
=> 내외적으로 TimeBox와 Bell을 함께 다루는 객체를 커찬이 이야기꺼냈던 것 같은데 현재 타임박스 안에 bell이 완전 종속관계로 따라다니다보니까 두 객체를 함께 다루어야 하고 + 반환형은 하나이므로 도메인 객체나 엔티티가 아닌 DTO를 반환하는 private 메서드를 만들 수 밖에 없다는 게 아쉬운 것 같아요.
- 역할적으로는 잘 이해되었으나 커찬이 말한 (타임박스 + 벨 리스트)를 묶는 단위 도메인 객체를 고려해봐도 좋을 듯 싶습니다.
|
|
||
| @Transactional(readOnly = true) | ||
| public CustomizeTableResponse findTable(long tableId, Member member) { | ||
| CustomizeTableEntity tableEntity = tableRepository.findByIdAndMember(tableId, member) |
There was a problem hiding this comment.
[제안]
일관성을 위해 getByIdAndMember로 만드는 건 어떤가요?
+) 밑에도 동일 로직이 세번 반복되어 있는데 적어도 private method 분리해도 좋을 것 같아요!
There was a problem hiding this comment.
요기도 getByIdAndMember 한번 적용해주세요!
| CustomizeTable table | ||
| ) { | ||
| List<CustomizeTimeBoxCreateRequest> timeBoxCreateRequests = tableCreateRequest.table(); | ||
| List<CustomizeTimeBoxResponse> timeBoxResponses = IntStream.range(0, timeBoxCreateRequests.size()) |
There was a problem hiding this comment.
이 코드 마음에 안드는데 익숙해서 어디서 봤나 했더니 내가 쓴 코드였네 ㅋㅋ
| .set("speechType", "입론1") | ||
| .set("boxType", CustomizeBoxType.NORMAL) | ||
| .set("time", 120) | ||
| .set("bell", getBellRequestBuilder().sampleList(2)) |
There was a problem hiding this comment.
어우 픽스쳐 몽키 사용하길 잘했다 ㅋㅋ
| ); | ||
|
|
||
| CustomizeTableResponse savedTableResponse = customizeService.save(customizeTableCreateRequest, chan); | ||
| Optional<CustomizeTableEntity> foundTable = customizeTableRepository.findById(savedTableResponse.id()); |
There was a problem hiding this comment.
isPresent() 없이 .get() 해서 검증할거면 굳이 Optional 필요없을 것 같은데 getByIdAndMember 쓰는건 어때요?
|
|
||
| customizeService.updateUsedAt(table.getId(), member); | ||
|
|
||
| Optional<CustomizeTableEntity> updatedTable = customizeTableRepository.findById(table.getId()); |
src/test/java/com/debatetimer/service/customize/CustomizeServiceV2Test.java
Show resolved
Hide resolved
leegwichan
left a comment
There was a problem hiding this comment.
/noti @unifolio0
일단 넘어가기로 했으니 넘어가시죠. Approve 했습니다.
더 자세한 이야기는 월요일에 진행하는 걸로
c.f. CustomizeServiceV2Test는 일부러 안만든 거죠?
|
|
||
| void delete(BellEntity bell); | ||
|
|
||
| void deleteAllByCustomizeTimeBoxIn(List<CustomizeTimeBox> customizeTimeBoxes); |
There was a problem hiding this comment.
낙낙 패거리에 비토가 포함되어 있는 거잖아요?
그렇다면 비토 패거리 아닐까요?
There was a problem hiding this comment.
서비스 로직이 길어지는 건 어쩔 수 없는 것 같아요. 일단 넘어가고, 월요일에 더 이야기해보죠
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/test/java/com/debatetimer/service/customize/CustomizeServiceV2Test.java (2)
36-60: 테스트 데이터 중복 개선 필요여러 테스트에서 동일한
CustomizeTableCreateRequest구조가 반복되고 있습니다. 테스트 가독성과 유지보수성을 높이기 위해 공통 테스트 데이터를 추출하는 것을 고려해보세요.다음과 같이 헬퍼 메서드를 추가하여 중복을 줄일 수 있습니다:
+ private CustomizeTableCreateRequest createSampleTableRequest() { + return new CustomizeTableCreateRequest( + new CustomizeTableInfoCreateRequest("자유 테이블", "주제", "찬성", + "반대", true, true), + List.of( + new CustomizeTimeBoxCreateRequest(Stance.PROS, "입론1", CustomizeBoxType.NORMAL, + 120, List.of(new BellRequest(90, 1)), 60, null, "발언자1"), + new CustomizeTimeBoxCreateRequest(Stance.PROS, "입론2", CustomizeBoxType.NORMAL, + 120, List.of(new BellRequest(90, 1), new BellRequest(120, 2)), 60, null, "발언자2") + ) + ); + }
193-202: 테스트 메서드 명칭 불일치 해결 완료 확인이전 리뷰에서 지적된 테스트 메서드 명칭과 실제 테스트 내용 불일치 문제가 해결되었습니다. 메서드명이
회원_소유가_아닌_테이블_수정_시_예외를_발생시킨다로 되어 있으나, 실제로는updateUsedAt메서드를 올바르게 테스트하고 있습니다.더 정확한 의미 전달을 위해 메서드명을 다음과 같이 수정하는 것을 고려해보세요:
- void 회원_소유가_아닌_테이블_수정_시_예외를_발생시킨다() { + void 회원_소유가_아닌_테이블_사용시각_업데이트_시_예외를_발생시킨다() {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/test/java/com/debatetimer/service/customize/CustomizeServiceTest.java(7 hunks)src/test/java/com/debatetimer/service/customize/CustomizeServiceV2Test.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/com/debatetimer/service/customize/CustomizeServiceTest.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-push
🔇 Additional comments (2)
src/test/java/com/debatetimer/service/customize/CustomizeServiceV2Test.java (2)
217-226: 삭제 테스트의 완전성 검증 우수함삭제 테스트가 연관된 모든 엔티티(테이블, 타임박스, 벨)의 삭제를 포괄적으로 검증하고 있어 캐스케이드 삭제 기능이 올바르게 작동하는지 확인할 수 있습니다.
28-32: 테스트 클래스 구조 및 의존성 주입 적절함중첩 클래스를 사용한 테스트 구조화와
@Autowired를 통한 서비스 의존성 주입이 적절하게 구성되어 있습니다.
| void 테이블_정보_수정을_동시에_요청할_때_동시에_처리하지_않는다() throws InterruptedException { | ||
| Member member = memberGenerator.generate("default@gmail.com"); | ||
| CustomizeTableEntity table = customizeTableGenerator.generate(member); | ||
| CustomizeTableCreateRequest request = new CustomizeTableCreateRequest( | ||
| new CustomizeTableInfoCreateRequest("자유 테이블", "주제", "찬성", | ||
| "반대", true, true), | ||
| List.of( | ||
| new CustomizeTimeBoxCreateRequest(Stance.PROS, "입론1", CustomizeBoxType.NORMAL, | ||
| 120, List.of(new BellRequest(90, 1)), 60, null, "발언자1"), | ||
| new CustomizeTimeBoxCreateRequest(Stance.PROS, "입론2", CustomizeBoxType.NORMAL, | ||
| 120, List.of(new BellRequest(90, 1), new BellRequest(120, 2)), 60, null, "발언자2") | ||
| ) | ||
| ); | ||
|
|
||
| runAtSameTime(2, () -> customizeService.updateTable(request, table.getId(), member)); | ||
|
|
||
| assertThat(customizeTimeBoxRepository.findAllByCustomizeTable(table)).hasSize(2); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
동시성 테스트 검증 강화 필요
동시성 테스트가 실제로 동시 실행을 보장하는지 검증이 필요합니다. 현재 테스트는 결과만 확인하고 있어 실제 동시성 제어가 작동하는지 확인하기 어렵습니다.
다음과 같은 개선사항을 고려해보세요:
@Test
void 테이블_정보_수정을_동시에_요청할_때_동시에_처리하지_않는다() throws InterruptedException {
Member member = memberGenerator.generate("default@gmail.com");
CustomizeTableEntity table = customizeTableGenerator.generate(member);
CustomizeTableCreateRequest request = createSampleTableRequest();
+
+ AtomicInteger successCount = new AtomicInteger(0);
+ AtomicInteger exceptionCount = new AtomicInteger(0);
- runAtSameTime(2, () -> customizeService.updateTable(request, table.getId(), member));
+ runAtSameTime(2, () -> {
+ try {
+ customizeService.updateTable(request, table.getId(), member);
+ successCount.incrementAndGet();
+ } catch (Exception e) {
+ exceptionCount.incrementAndGet();
+ }
+ });
- assertThat(customizeTimeBoxRepository.findAllByCustomizeTable(table)).hasSize(2);
+ assertAll(
+ () -> assertThat(customizeTimeBoxRepository.findAllByCustomizeTable(table)).hasSize(2),
+ () -> assertThat(successCount.get()).isEqualTo(2),
+ () -> assertThat(exceptionCount.get()).isEqualTo(0)
+ );
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/test/java/com/debatetimer/service/customize/CustomizeServiceV2Test.java
around lines 154 to 171, the concurrency test does not verify that the
updateTable method is actually executed concurrently, only checking the final
result. To fix this, enhance the test by adding synchronization mechanisms such
as latches or barriers to ensure the two updateTable calls run simultaneously.
Also, consider adding assertions or logs inside the concurrent execution block
to confirm that concurrent access occurs, thereby validating the concurrency
control effectively.
|
/noti |
coli-geonwoo
left a comment
There was a problem hiding this comment.
/noti
비토 늦었어유 ㅇㅁㅇ
getByIdAndMember를 만들어놓았는데 사용하지 않는 곳이 있어서 이것만 확인부탁할게요! 고생많았습니다~
|
|
||
| @Transactional(readOnly = true) | ||
| public CustomizeTableResponse findTable(long tableId, Member member) { | ||
| CustomizeTableEntity tableEntity = tableRepository.findByIdAndMember(tableId, member) |
There was a problem hiding this comment.
요기도 getByIdAndMember 한번 적용해주세요!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/com/debatetimer/service/customize/CustomizeService.java (1)
36-36: 코드 단순화가 좋지만 클래스 내 일관성을 개선해주세요.
getByIdAndMember직접 호출로 변경한 것은 코드를 단순화하고updateTable메서드와 일관성을 맞춘 좋은 변경입니다.하지만 동일한 클래스 내의
updateUsedAt(58-59번 줄)과deleteTable(68-69번 줄) 메서드들도 동일한 패턴으로 변경하는 것을 고려해보세요.다음과 같이 일관성을 맞출 수 있습니다:
// updateUsedAt 메서드에서 - CustomizeTableEntity tableEntity = tableRepository.findByIdAndMember(tableId, member) - .orElseThrow(() -> new DTClientErrorException(ClientErrorCode.TABLE_NOT_FOUND)); + CustomizeTableEntity tableEntity = tableRepository.getByIdAndMember(tableId, member); // deleteTable 메서드에서 - CustomizeTableEntity table = tableRepository.findByIdAndMember(tableId, member) - .orElseThrow(() -> new DTClientErrorException(ClientErrorCode.TABLE_NOT_FOUND)); + CustomizeTableEntity table = tableRepository.getByIdAndMember(tableId, member);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/com/debatetimer/service/customize/CustomizeService.java(1 hunks)src/main/java/com/debatetimer/service/customize/CustomizeServiceV2.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/debatetimer/service/customize/CustomizeServiceV2.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-push
🚩 연관 이슈
closed #190
🗣️ 리뷰 요구사항 (선택)
어차피 바뀔꺼라 그냥 메소드 길이 무시하고 한 곳에 몰아넣었습니다.
Summary by CodeRabbit
신규 기능
버그 수정
문서화
테스트
기타